Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(reports): allow API requests to filter rules to evaluate #977

Merged
merged 12 commits into from
Jun 7, 2022

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Jun 2, 2022

Resolves #918

Right now, I am wondering how I should have the optional String filter be used in the API, because there are a lot of methods and steps to go through receiving the request from the client, to going through execs, to finally sending a post request.

I have two options right now as to how I should structure the optional filter string.

  1. Should we overload all the methods such that we are able to specify an optional filter String as a parameter? The benefits of this I see are that it's easy to see if we are including "filters" in a request since a argument wouldn't be supplied, but the drawback is that it's doubling the methods that are needed to go through the process.

  2. The second option is what I am doing right now in the code, where I am supplying a filter String parameter for all the methods that need it and I only check at the very end, (in this case: exec in RemoteReportGenerator) if the String is empty or not. If the string was empty, then we just send a POST request normally without the query string, if it wasn't etc. etc....

  3. An unrelated question, but in SubprocessReportGenerator the class directly uses [cryostat-reports]'s InterruptibleReportGenerator method in ReportResource.java which I updated in the last pr. And in that class, I use I made my own predicate combiner thing, and I also use it in SubprocessReportGenerator by making a new util function. Is this the right approach so that I will eventually refactor the [cryostat-reports] class to use the util function as well?

Thanks!

maxcao13 and others added 5 commits June 2, 2022 19:35
* test(graphql): add basic targetNodes query itest

* test(graphql): add targetNodes test with filter

* test(graphql): add more itests

Add tests for starting recording and querying environmentNodes

* test(graphql): add archive/delete itests

* remove println
* build(oci): transform image tag to lower case

* Update CI as well
@maxcao13 maxcao13 added the feat New feature or request label Jun 2, 2022
@andrewazores
Copy link
Member

Approach 2 sounds good to me. No need to have overloaded versions for with- and without-filters - just let the filter String be nullable, or Optional.

An unrelated question, but in SubprocessReportGenerator the class directly uses [cryostat-reports]'s InterruptibleReportGenerator method in ReportResource.java which I updated in the last pr. And in that class, I use I made my own predicate combiner thing, and I also use it in SubprocessReportGenerator by making a new util function. Is this the right approach so that I will eventually refactor the [cryostat-reports] class to use the util function as well?

Ah right, makes sense. I should have thought ahead to that point when reviewing your -core and -reports changes.

It's OK to add the util function and have it as essentially duplicate code across the repos for now. Later on we can extract that logic out into a util in -core so that both -reports and this project can both reuse it.

@maxcao13 maxcao13 requested a review from andrewazores June 6, 2022 20:59
@maxcao13
Copy link
Member Author

maxcao13 commented Jun 6, 2022

Feedback please :)

requests with filters are added with a query string ?filter="[filter1],[filter2],..." on the ReportGetHandler and TargetReportGetHandler

Here is a small messy rundown of what happens with each type filter query
when filter=""
- ALL rules; filter = {}

when no filter (Attribute is not given)
- ALL rules; filter = {null}

when filter="[badfilter, badfilter2]"
- NO RULES; filter = {[badfilter, badfilter2]}

@maxcao13
Copy link
Member Author

maxcao13 commented Jun 7, 2022

I will remove prints once OK'd.

Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, nice work.

@maxcao13 maxcao13 marked this pull request as ready for review June 7, 2022 20:51
@andrewazores andrewazores merged commit bb83001 into cryostatio:main Jun 7, 2022
@maxcao13 maxcao13 deleted the n-br branch June 9, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] API requests should be able to specify which rules to evaluate
3 participants